Client-side violations for money request updates #34402

lindboe · 2024-01-12T01:05:49Z

Details

Adds optimistic client-side updates for money requests for missing tags, categories, or disabled tags/categories. This PR handles edits, while #32528 handled creation of money requests. Requires setting up a policy in old dot and configuring it to have a workspace (see testing instructions). This PR does not make any visible changes; that is scoped to other tickets.

Fixed Issues

$ #31093
PROPOSAL:

Tests

Verify that no errors appear in the JS console

Setup:

Follow the instructions here to set up a policy with a workspace in New Dot.
Within the policy in Old Dot, navigate Settings > Categories and verify that People must categorize expenses is enabled, and make sure there's at least two categories on the policy
Within the policy in Old Dot, navigate Settings > Tags and verify that People must tag expenses is enabled, and make sure there's at least two tags enabled on the policy

Instructions for all tests

Expectations for violations, generally:

Violations should be visible when viewing the request on each menu item with text and a RBR indicator. Here, we're generally concerned the following violations that we calculate client-side: missing category, missing tag, category out of policy, and tag out of policy. Other visible violations should not be affected by this PR.

Testing the violations were stored correctly (web only):

Navigate to the ONYXDB -> keyvaluepairs within your browser storage tab
Identify the transaction that was created with the dollar amount you specified, should look like transactions_someID. If you open chrome dev tools before making the money request, you can inspect the response which should have the transactionID
Verify there's a transaction violations object with the corresponding ID like transactionViolations_SameIDasTransaction, with the array content specified by the test

Test no client-side violations

Create a request and request money from the policy you created in setup. Set an amount, merchant, category, and tag. Ensure your request amount is not above the maximum set by the policy, and not above the amount the policy requires for a receipt.
(Web) Once the request is created, check your browser storage tab. You should see either no matching transactionViolations_ID key, or, if you've navigated to edit the request, you may see a transaction violations list with only a cashExpenseWithNoReceipt violation.
Edit the request's date and save. There should be no visible errors on the request or the LHN.
(Web) inspect the transaction violations key again. The violations list should still only contain a cashExpenseWithNoReceipt violation.

Test missing tag

Create a request and request money from the policy you created in setup. Set an amount, merchant, and category, but DO NOT set tag. Save the request.
(Web) inspect the transaction violations object. You should see a matching transactionViolations_ID key, with an array containing a missingTag violation, and possibly also a cashExpenseWithNoReceipt violation.
Edit the request to add a tag. There should be no visible errors on the request or the LHN.
1. (Web) inspect the transaction violations key again. You should see either no matching transactionViolations_ID key, or, if you've navigated to edit the request, you may see a transaction violations list with only a cashExpenseWithNoReceipt violation.

Test missing category

Create a request and request money from the policy you created in setup. Set an amount, merchant, and tag, but DO NOT set category. Save the request.
(Web) inspect the transaction violations object. You should see a matching transactionViolations_ID key, with an array containing a missingCategory violation, and possibly also a cashExpenseWithNoReceipt violation.
Edit the request to add a category. There should be no visible errors on the request or the LHN.
1. (Web) inspect the transaction violations key again. You should see either no matching transactionViolations_ID key, or, if you've navigated to edit the request, you may see a transaction violations list with only a cashExpenseWithNoReceipt violation, or no violations.

Test tag out of policy

Create a request and request money from the policy you created in setup. Set an amount, merchant, and tag and category that are enabled on old dot. Save the request.
On old dot, disable the tag you used. Refresh new dot, and ensure a tagOutOfPolicy violation is shown in the transaction violations list.
Edit the request to use a tag that is enabled in the policy, and ensure that tagOutOfPolicy is no longer in the transaction violations list.

Test category out of policy

Create a request and request money from the policy you created in setup. Set an amount, merchant, and tag and category that are enabled on old dot. Save the request.
On old dot, disable the category you used. Refresh new dot, and ensure a categoryOutOfPolicy violation is shown in the transaction violations list.
Edit the request to use a category that is enabled in the policy, and ensure that categoryOutOfPolicy is no longer in the transaction violations list.

Offline tests

Repeat the tests outlined above while completely offline, except for "tag/category out of policy", because you can't change policy while offline. Different steps for those tests are provided below.

Then, in addition, after each test, go online and verify the transactionViolations key remains the same, with the potential addition of a cashExpenseWithNoReceipt violation, or no violations.

Test tag out of policy (offline)

Create a request and request money from the policy you created in setup. Set an amount, merchant, and tag and category that are enabled on old dot. Save the request.
On old dot, disable the tag you used. Refresh new dot, and ensure a tagOutOfPolicy violation is shown in the transaction violations list.
On new dot, go offline. Select a tag that has remained enabled on your policy on the request instead. Save the request, and ensure the transaction violations list no longer has a tagOutOfPolicy violation.
Go online, and ensure that the transaction violations list does not change.

Test category out of policy (offline)

Create a request and request money from the policy you created in setup. Set an amount, merchant, and tag and category that are enabled on old dot. Save the request.
On old dot, disable the category you used. Refresh new dot, and ensure a categoryOutOfPolicy violation is shown in the transaction violations list.
On new dot, go offline. Select a category that has remained enabled on your policy on the request instead. Save the request, and ensure the transaction violations list no longer has a categoryOutOfPolicy violation.
Go online, and ensure that the transaction violations list does not change.

Test failure to edit request (web only, requires Chrome)

Create a request and request money from the policy you created in setup. Set an amount, merchant, and category, and tag. Save the request.
Go to old dot and disable the category you used.
Refresh new dot.
Inspect the corresponding transactionViolations_ key to ensure there is a categoryOutOfPolicy violation there.
Now, cause the request to the backend to fail:
With the Network tab open in Chrome devtools, edit a DIFFERENT request's category. Look for a request entry matching api?command=EditMoneyRequest (it is possible that instead it might be a more specific command in the future, like UpdateMoneyRequestTag). Right-click and select "Block request URL". This will cause subsequent calls to this URL to fail.
Go offline and edit the request to use an enabled category. Save the request, and inspect the transaction violations key. The categoryOutOfPolicy violation should be gone.
Go online and allow the request to be sent and fail.
Wait a little bit for the request to fail after several retries (you should see an error in the transaction thread), and verify that the transaction violations list in Onyx is reset to what it was before you fixed the request (i.e., it contains a categoryOutOfPolicy violation).
Unblock the endpoint you blocked before, and again update the request to use an enabled category. The transactionViolations list should be updated to remove the categoryOutOfPolicy violation.

QA Steps

Verify that no errors appear in the JS console

Same as tests

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

Tag out of policy tests

Category out of policy tests

Failure data tests

MacOS: Desktop

Update everywhere that accesses an arg on policy to use {} as default arg for safety, since we can't use optional chaining

…money-request-updates

…saction violations. Fix and make that more clear with argument names

src/libs/ViolationsUtils.ts

…money-request-updates

melvin-bot · 2024-01-17T00:10:43Z

@parasharrajat Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

…money-request-updates

lindboe · 2024-01-21T00:03:04Z

@parasharrajat Hi, are you going to be able to review this PR soon?

parasharrajat · 2024-01-23T16:47:53Z

@lindboe Please merge main. Reviewing it now.

lindboe · 2024-01-23T19:50:25Z

@parasharrajat Looks like a significant change from upstream -- the editMoneyRequest command was removed entirely in favor of the more detailed, specific commands like updateMoneyRequestAmountAndCurrency. Easy to fix conflict-wise, but this does mean it has changed what command is used since it was last tested, so just FYI.

Fixing conflicts now...

…money-request-updates

This reverts commit 3493040.

lindboe · 2024-02-06T19:24:07Z

Okay, so that this can move forward, I'm aiming for minimal changes:

I un-deleted EditMoneyRequest since there's a deletion test now using it
I added a @ts-expect-error to the type issue in MoneyRequestView with the conflict between the component's typing as PolicyTags (not correct type for what the data returns and the ViolationsUtils function's use of PolicyTagList). I'll make a more clear report of this and share it in open-source so someone can work on fixing the policy tags typing and utils.
I changed the policy* defaultProps to null where TypeScript was complaining because OnyxEntry<Policy*> = Policy* | null, but defaults props was returning an empty object, and this difference was leading to some very confusing type conflicts. Based on my conversation here, null is the expected "empty" data type once components are typescript, so may as well change it now instead of trying to handle both everywhere.

Going to run through some test cases and make sure it's all good now.

lindboe · 2024-02-06T20:05:12Z

Okay @parasharrajat this should be good to go again, except the perf-tests that appear to be failing on main as well. Should have a message for open-source about the broken policy tags types in a little while.

lindboe · 2024-02-06T23:59:54Z

Asked about policyTags types here: https://expensify.slack.com/archives/C01GTK53T8Q/p1707263531033829

cead22 · 2024-02-07T00:48:12Z

@cead22 do you have input on a preferred approach here?

Thanks, I was going to suggest what you already did (to ignore the type errors for now, since the more we add to this PR the harder it'll get to get it merged)

…money-request-updates

lindboe · 2024-02-07T18:06:32Z

Another day, another refactor update: the command to update description now lives in its own component. Updated.

@cead22 while testing this update, I noticed an assumption in earlier decisions doesn't seem to be true: in a few places, we're checking if policy?.id exists, because "if it's a P2P request, there won't be a policy, and so we should skip trying to calculate transaction violations". That may have been true at one point, but isn't now, I do see a policy when I make a P2P request:

{
    "isFromFullPolicy": false,
    "id": "B88059F8975D1B23",
    "name": "Lizzi's Expenses",
    "role": "...",
    "type": "personal",
    "owner": "...",
    "ownerAccountID": ...,
    "outputCurrency": "USD",
    "avatar": "",
    "employeeList": [],
    "isPolicyExpenseChatEnabled": false,
    "lastModified": ...,
    "chatReportIDAnnounce": 0,
    "chatReportIDAdmins": 0
}

I'm assuming isFromFullPolicy or type are fields we should be checking against instead?

lindboe · 2024-02-08T00:27:26Z

Above issue with policy?.id discussed and resolved here: https://expensify.slack.com/archives/C01GTK53T8Q/p1707339619099769.

@parasharrajat tests are passing again. Just to clarify, next steps were for you to test this out again right?

parasharrajat · 2024-02-08T21:57:53Z

Screenshots

🔲 iOS / native

Screen.Recording.2024-02-09.at.2.51.16.PM.mov

🔲 iOS / Safari

Screen.Recording.2024-02-09.at.2.31.25.PM.mov

🔲 MacOS / Desktop

Screen.Recording.2024-02-09.at.2.25.42.PM.mov

🔲 MacOS / Chrome

Screen.Recording.2024-02-09.at.1.01.29.PM.mov

🔲 Android / Chrome

Screen.Recording.2024-02-09.at.2.34.42.PM.mov

🔲 Android / native

Screen.Recording.2024-02-09.at.2.50.14.PM.mov

parasharrajat

LGTM. There are couple of prop warnings but I think those will be removed when these components are migrated to TS.

I will also report some to Slack in sometime.

…money-request-updates

src/libs/Violations/ViolationsUtils.ts

Co-authored-by: Carlos Alvarez <cead22@gmail.com>

OSBotify · 2024-02-09T19:00:59Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-02-12T14:19:58Z

🚀 Deployed to staging by https://github.com/cead22 in version: 1.4.40-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	failure ❌

mountiny · 2024-02-13T18:01:38Z

Noting here that QA did not really validate this PR as the QA steps were not clear enough for them it seems like. I believe its fine not to block deploy on this and we will internally test these and report any issues as we find them

OSBotify · 2024-02-13T18:25:19Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.4.40-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	failure ❌

allgandalf · 2024-07-10T08:34:03Z

src/libs/actions/IOU.ts

@@ -1210,78 +1218,153 @@ function getUpdateMoneyRequestParams(
        });
    }

+    if (policy?.id && updatedTransaction) {


This incomplete condition caused #43767.

We should have also checked whether transactionChanges are made either for category or tag and not execute this check for every change in the transaction

lindboe added 11 commits January 9, 2024 22:39

Add optimistic violations to money request edits

a551927

Update everywhere that accesses an arg on policy to use {} as default arg for safety, since we can't use optional chaining

Merge remote-tracking branch 'upstream/main' into lindboe/violations/…

5ea3000

…money-request-updates

Lint fix

56dd74d

Fix bugs

fe4b178

Fix data passing

a189970

Temp fix for tag issue

69ad695

Use full policyTags data as ViolationsUtils input

41e87ca

getViolationsOnyxData requires new transaction data but previous tran…

a09e4ac

…saction violations. Fix and make that more clear with argument names

Improve types

72cfa45

Fix case of passing empty object to policyTags

8ff7c69

We should only run getViolationsOnyxData if there's a policy

f2942ea

lindboe force-pushed the lindboe/violations/money-request-updates branch from 6ad58a5 to f2942ea Compare January 12, 2024 23:40

lindboe commented Jan 12, 2024

View reviewed changes

src/libs/ViolationsUtils.ts Outdated Show resolved Hide resolved

lindboe added 4 commits January 15, 2024 13:14

Feedback: Don't use Maybe type

37bbacc

Ensure an array is always passed to getViolationsOnyxData

0a3e818

Merge remote-tracking branch 'upstream/main' into lindboe/violations/…

d211324

…money-request-updates

More prettier changes

ede3bbe

lindboe marked this pull request as ready for review January 17, 2024 00:10

lindboe requested a review from a team as a code owner January 17, 2024 00:10

melvin-bot bot requested review from parasharrajat and removed request for a team January 17, 2024 00:10

lindboe changed the title ~~[WIP] Client-side violations for money requests~~ Client-side violations for money requests Jan 17, 2024

lindboe changed the title ~~Client-side violations for money requests~~ Client-side violations for money request updates Jan 17, 2024

lindboe mentioned this pull request Jan 18, 2024

Adds new ReportUtils for Violations #31448

Merged

48 tasks

Merge remote-tracking branch 'upstream/main' into lindboe/violations/…

87d2f07

…money-request-updates

Merge remote-tracking branch 'upstream/main' into lindboe/violations/…

04fcb21

…money-request-updates

lindboe added 2 commits February 6, 2024 11:12

Random lint change

934bab4

Revert "editMoneyRequest is no longer used, delete"

fbbcdae

This reverts commit 3493040.

lindboe force-pushed the lindboe/violations/money-request-updates branch from 58d9c3d to fbbcdae Compare February 6, 2024 19:19

lindboe added 2 commits February 7, 2024 09:57

Merge remote-tracking branch 'upstream/main' into lindboe/violations/…

1eebb39

…money-request-updates

Update command in new description component

30a2f24

cead22 mentioned this pull request Feb 8, 2024

Clean up code assuming a money request is P2P when policy isn't set #36169

Closed

parasharrajat approved these changes Feb 9, 2024

View reviewed changes

melvin-bot bot requested a review from cead22 February 9, 2024 09:23

cdanwards mentioned this pull request Feb 9, 2024

Adds 'Required' label for merchant on workspaces & updates P2P requirements for worskpaces #36252

Merged

62 tasks

Merge remote-tracking branch 'upstream/main' into lindboe/violations/…

9cbb446

…money-request-updates

cead22 previously approved these changes Feb 9, 2024

View reviewed changes

src/libs/Violations/ViolationsUtils.ts Outdated Show resolved Hide resolved

src/libs/Violations/ViolationsUtils.ts Outdated Show resolved Hide resolved

Apply suggestions from code review

d528580

Co-authored-by: Carlos Alvarez <cead22@gmail.com>

lindboe dismissed cead22’s stale review via d528580 February 9, 2024 18:46

cead22 approved these changes Feb 9, 2024

View reviewed changes

cead22 merged commit 1923870 into Expensify:main Feb 9, 2024
16 checks passed

github-actions bot mentioned this pull request Feb 12, 2024

Deploy Checklist: New Expensify 2024-02-12 #36335

Closed

90 tasks

melvin-bot bot mentioned this pull request Feb 13, 2024

[$250] [HOLD for payment 2024-02-20] Implement Client-side Violations when updating money requests #31093

Closed

allgandalf reviewed Jul 10, 2024

View reviewed changes

allgandalf mentioned this pull request Jul 10, 2024

[HOLD for payment 2024-07-10] [$250] Category - "Category no longer valid" shows up briefly after changing amount #43767

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client-side violations for money request updates #34402

Client-side violations for money request updates #34402

lindboe commented Jan 12, 2024 •

edited by cead22

Loading

melvin-bot bot commented Jan 17, 2024

lindboe commented Jan 21, 2024

parasharrajat commented Jan 23, 2024

lindboe commented Jan 23, 2024

lindboe commented Feb 6, 2024 •

edited

Loading

lindboe commented Feb 6, 2024 •

edited

Loading

lindboe commented Feb 6, 2024

cead22 commented Feb 7, 2024

lindboe commented Feb 7, 2024 •

edited

Loading

lindboe commented Feb 8, 2024

parasharrajat commented Feb 8, 2024 •

edited

Loading

parasharrajat left a comment

OSBotify commented Feb 9, 2024

OSBotify commented Feb 12, 2024

mountiny commented Feb 13, 2024

OSBotify commented Feb 13, 2024

allgandalf Jul 10, 2024

Client-side violations for money request updates #34402

Client-side violations for money request updates #34402

Conversation

lindboe commented Jan 12, 2024 • edited by cead22 Loading

Details

Fixed Issues

Tests

Setup:

Instructions for all tests

Test no client-side violations

Test missing tag

Test missing category

Test tag out of policy

Test category out of policy

Offline tests

Test tag out of policy (offline)

Test category out of policy (offline)

Test failure to edit request (web only, requires Chrome)

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Jan 17, 2024

lindboe commented Jan 21, 2024

parasharrajat commented Jan 23, 2024

lindboe commented Jan 23, 2024

lindboe commented Feb 6, 2024 • edited Loading

lindboe commented Feb 6, 2024 • edited Loading

lindboe commented Feb 6, 2024

cead22 commented Feb 7, 2024

lindboe commented Feb 7, 2024 • edited Loading

lindboe commented Feb 8, 2024

parasharrajat commented Feb 8, 2024 • edited Loading

Screenshots

🔲 iOS / native

🔲 iOS / Safari

🔲 MacOS / Desktop

🔲 MacOS / Chrome

🔲 Android / Chrome

🔲 Android / native

parasharrajat left a comment

Choose a reason for hiding this comment

OSBotify commented Feb 9, 2024

OSBotify commented Feb 12, 2024

mountiny commented Feb 13, 2024

OSBotify commented Feb 13, 2024

allgandalf Jul 10, 2024

Choose a reason for hiding this comment

lindboe commented Jan 12, 2024 •

edited by cead22

Loading

lindboe commented Feb 6, 2024 •

edited

Loading

lindboe commented Feb 6, 2024 •

edited

Loading

lindboe commented Feb 7, 2024 •

edited

Loading

parasharrajat commented Feb 8, 2024 •

edited

Loading